Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<xstring>: Strengthen exception specification for sv.compare("NTBS") #3738

Merged
merged 7 commits into from
Jun 15, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented May 28, 2023

As discovered in #3735, both

  • basic_string_view's constructor from a const _Elem* (strengthened) and
  • the basic_string_view::compare overload for two basic_string_views (mandatory)

are already noexcept, so basic_string_view::compare for a basic_string_view and a const _Elem* should also be noexcept.

The exception specification strengthening is already done for basic_string.

STL/stl/inc/xstring

Lines 4646 to 4649 in a621095

_NODISCARD _CONSTEXPR20 int compare(_In_z_ const _Elem* const _Ptr) const noexcept /* strengthened */ {
// compare [0, size()) with [_Ptr, <null>)
return _Traits_compare<_Traits>(_Mypair._Myval2._Myptr(), _Mypair._Myval2._Mysize, _Ptr, _Traits::length(_Ptr));
}

The message in the static_assert "(this is bad, char_traits)" no longer holds, since the exception specifications of all char_traits::compare in MSVC STL are already strengthened.


Driven-by changes: make basic_string::_Eos, basic_string::_Swap_proxy_and_iterators, and basic_string::_Swap_data noexcept since they are non-throwing and already called by noexcept functions.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner May 28, 2023 01:40
Since they are already called by `noexcept` functions.
@frederick-vs-ja
Copy link
Contributor Author

frederick-vs-ja commented May 28, 2023

I think we should do strengthening for _Eos, _Swap_proxy_and_iterators, and _Swap_data since they are already called by noexcept functions.

I'm hesitant about other functions, because

  • _Move_assign_from_buffer and _Copy_assign_val_from_small are only called from functions that may throw an exception,
  • reserve(void) is deprecated, and other invocations of _Become_small are only from functions that may throw an exception (like the above).

@achabense
Copy link
Contributor

achabense commented May 28, 2023

_Move_assign_from_buffer:
It doesn't involve memory allocation or other throwable operations.
The caller sstream::str()&& (here) is throwable due to assignment in another place.

_Copy_assign_val_from_small:
It doesn't involve memory allocation or other throwable operations.
The caller operator=(const basic_string&) (here) is throwable due to assignment in another place.
(off topic) I saw // TRANSITION, VSO-761321; inline into only caller when that's fixed in its code. What is it? Can it be fixed now?

_Become_small:
It's used to deallocate superfluous memory. Compared with _Tidy_deallocate (here) I believe it's safe to add noexcept.
The callers are reserves. They are throwable due to some potential re-allocations. As to reserve(void), it doesn't involve such kind of re-allocation. Though deprecated, a noexcept /*strengthened*/ is very cheap to add so I think it's ok.

(off topic)
basic_string(nullptr_t) is now inserted between basic_string(const _Elem*) and basic_string(const _Elem*, const _Alloc& ). The latter two work as a whole to resolve as basic_string(const _Elem*, const _Alloc& = _Alloc()). For better maintainability, please consider moving basic_string(nullptr_t) elsewhere.

STL/stl/inc/xstring

Lines 2539 to 2553 in a621095

_CONSTEXPR20 basic_string(_In_z_ const _Elem* const _Ptr) : _Mypair(_Zero_then_variadic_args_t{}) {
_Construct<_Construct_strategy::_From_ptr>(_Ptr, _Convert_size<size_type>(_Traits::length(_Ptr)));
}
#if _HAS_CXX23
basic_string(nullptr_t) = delete;
#endif // _HAS_CXX23
#if _HAS_CXX17
template <class _Alloc2 = _Alloc, enable_if_t<_Is_allocator<_Alloc2>::value, int> = 0>
#endif // _HAS_CXX17
_CONSTEXPR20 basic_string(_In_z_ const _Elem* const _Ptr, const _Alloc& _Al)
: _Mypair(_One_then_variadic_args_t{}, _Al) {
_Construct<_Construct_strategy::_From_ptr>(_Ptr, _Convert_size<size_type>(_Traits::length(_Ptr)));
}

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label May 28, 2023
@CaseyCarter CaseyCarter self-assigned this Jun 7, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jun 14, 2023
@StephanTLavavej
Copy link
Member

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

tests/std/tests/P0220R1_string_view/test.cpp Outdated Show resolved Hide resolved
@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Jun 14, 2023

@StephanTLavavej minor change pushed

@StephanTLavavej
Copy link
Member

@strega-nil-ms I believe you have flipped the sense of the message; compare the previous message.

(The message is admittedly confusing about whether it's stating what is currently observed, or what should be observed.)

@StephanTLavavej
Copy link
Member

@strega-nil-ms @CaseyCarter I've pushed changes to restore the // strengthened comment, then mechanically replace the entire test to use terse static_assert as it was already doing in several places (yay for C++17). For the test suite, static_assert messages are much less useful than in product code, so I didn't restrict this replacement to just the noexcept stuff, I did the whole file.

@StephanTLavavej StephanTLavavej merged commit 1573c78 into microsoft:main Jun 15, 2023
@StephanTLavavej
Copy link
Member

🦾 ⚠️ 🧶

@frederick-vs-ja frederick-vs-ja deleted the noexcept-compare-ntbs branch June 15, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants